-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Credentials also supporting symmetric keys #294
Credentials also supporting symmetric keys #294
Conversation
will support more types of credentials, including different types of COSE_Key's the handling of id_cred's is inspired by chrysn's PR openwsn-berkeley#274
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ElsaLopez133 thanks for pushing on parse_ccs_psk. Some comments below.
Please format the code with cargo fmt
(or even better, install the pre-commit tool to do that automatically -- see the file .pre-commit-config.yaml).
// Why do they add +3 and +1 in CredentialPRK::parse
This comment can be sent over e.g. slack rather than in the code
Also please use a commit message following the pattern we use in the project (see previous commits to get a sense of it).
ff9c761
to
4f2348f
Compare
borrowing code from @chrysn's PR openwsn-berkeley#274
3a5b5de
to
85b4875
Compare
Not all tests pass yet, but this is now ready for review:
In terms of code, something that still bothers me is the overuse of Also as stated in the first comment, this borrows ideas and code from #274. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats for pulling this one off! What remains to be done for this to be in a state where we can merge it? I left some comments inline, mainly for my understanding.
initiator | ||
.set_identity( | ||
I.try_into().expect("Wrong length of initiator private key"), | ||
cred_i.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to clone cred_i
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because we need access to cred_i later in credential_check_or_fetch(Some(cred_i), id_cred_i)
.
(but maybe this could be a reference, will take a look)
// compute ciphertext_2 | ||
let plaintext_2 = encode_plaintext_2(c_r, &id_cred_r, &mac_2, &ead_2)?; | ||
let plaintext_2 = encode_plaintext_2(c_r, id_cred_r.as_encoded_value(), &mac_2, &ead_2)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little confused with a difference between id_cred.as_full_value()
and id_cred.as_encoded_value()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit with better documentation on both functions, hope it suffices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
} | ||
} | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you comment on why we don't need this block any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Section 5.4.3 of RFC 9528, ID_CRED_I is just made available to the application, which will process it. So even if it is a credential by value, we don't need to parse it here.
In addition, now the logic to parse ID_CRED_I is embedded in decode_plaintext_3
, which calls IdCred::from_encoded_value
. That's why we don't need to use different constructs for e.g. CompactKid
or FullCredential
.
Note that this only parses ID_CRED_I: from a (potentially compact-encoded) reference or value, e.g. kid
/ {14: kccs}
, to the actual value of the ID_CRED_I, e.g.{4: kid}
/ {14: kcss}
. Then, resolving the kid
to a credential or verifying the syntax of the kcss
is left to the application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for the explanation!
shared/src/cred.rs
Outdated
/// Creates a new CCS credential with the given bytes and a pre-shared key | ||
/// | ||
/// This type of credential is to be used with the under-development EDHOC method PSK. | ||
pub fn new_ccs_psk(bytes: BufferCred, symmetric_key: BytesKeyAES128) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this in the current PR?
} else { | ||
Err(EDHOCError::ParsingError) | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the PSK parsing routine should be added on top of the current PR as a separate branch, which we can merge once the PSK method becomes more stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I documented it as experimental and moved the tests to a dedicated test_experimental
module.
cd97b1b
into
openwsn-berkeley:crypto-method-agility
includes breaking changes from PRs openwsn-berkeley#284 and openwsn-berkeley#294
Work in progress. Leverages some ideas from #274.
Built on top of #284, and supposed to be merged into the feature branch
crypto-method-agility
.